-
Notifications
You must be signed in to change notification settings - Fork 1.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Making force merge threadpool 1/8th of total cores #17255
base: main
Are you sure you want to change the base?
Conversation
Signed-off-by: Gaurav Bafna <[email protected]>
ffa411f
to
abc0a90
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #17255 +/- ##
============================================
- Coverage 72.43% 72.39% -0.04%
- Complexity 65725 65728 +3
============================================
Files 5318 5318
Lines 305675 305677 +2
Branches 44350 44350
============================================
- Hits 221408 221304 -104
- Misses 66055 66207 +152
+ Partials 18212 18166 -46 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice! This will be a big improvement for customers that have trouble scaling up that depend on force merges
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, much needed. Should we also make it dynamic?
OpenSearch Threadpools are already dynamic now starting 2.17 . Reference |
Signed-off-by: Gaurav Bafna <[email protected]>
❌ Gradle check result for 846824f: FAILURE Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change? |
With dynamic I understand we can change it without restart. But to set the default behavior is there any benchmark we have to understand the improvement with increasing the threadpool size in terms of disk IOPS utilization and force merge time reduction ? My question would be why 1/8th and not 1/4th or 1/16th as default ? |
Description
Currently force merge threads are bounded to size of 1 , irrespective of total cores available. This makes the force merges very slow and doesn't scale it with number of cores
This PR increases the thread count to 1/8th of the total cores, thereby making the force merges to run faster, still capped at 12.5% of overall CPU available.
Related Issues
Resolves #[Issue number to be closed when this PR is merged]
Check List
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.